[SPARK-57555][FOLLOWUP][SQL] Add escape-hatch conf to keep legacy JDBC TIME-to-timestamp mapping#56884
[SPARK-57555][FOLLOWUP][SQL] Add escape-hatch conf to keep legacy JDBC TIME-to-timestamp mapping#56884cloud-fan wants to merge 2 commits into
Conversation
…C TIME-to-timestamp mapping ### What changes were proposed in this pull request? SPARK-57555 made the built-in JDBC data source read SQL `TIME` columns as `TimeType` (instead of the legacy `TimestampType`) when `spark.sql.timeType.enabled` is true. That is a schema-inference change for existing JDBC reads: a column that previously came back as `TimestampType` now comes back as `TimeType`, with no DDL change by the user. This PR adds an internal escape-hatch conf, `spark.sql.legacy.jdbc.timeMapping.enabled` (default `false`), following the existing `spark.sql.legacy.<db>.*Mapping.enabled` family. When set to true, JDBC `TIME` columns keep the legacy `TimestampType` mapping even when `spark.sql.timeType.enabled` is true. It has no effect when `spark.sql.timeType.enabled` is false. ### Why are the changes needed? `spark.sql.timeType.enabled` is a single global switch that governs `TIME` support across all of Spark. Enabling it flips JDBC `TIME` reads from `TimestampType` to `TimeType` in one step, which can silently break workloads that depend on the old mapping (downstream casts, comparisons, stored schemas, serialization). The escape hatch lets such workloads opt back into the legacy JDBC behavior without having to keep the entire `TIME` type disabled. ### Does this PR introduce any user-facing change? No behavioral change by default. The new conf defaults to `false`, preserving SPARK-57555 behavior. When explicitly set to `true` (and `spark.sql.timeType.enabled` is `true`), JDBC `TIME` columns read as `TimestampType` as they did before SPARK-57555. ### How was this patch tested? Added a unit test in `JDBCSuite` asserting that, with `spark.sql.timeType.enabled=true`, the column reads as `TimestampType` when the escape hatch is on and as `TimeType` when it is off. ### Was this patch authored or co-authored using generative AI tooling? No
|
cc @MaxGekk |
|
I reviewed the change and didn't find correctness issues. The config semantics look right to me: JDBC I also checked that dialect-specific mappings such as PostgreSQL Only minor note: the new test does not explicitly cover |
…mapping conf and cover disabled-TIME case Add `.withBindingPolicy(ConfigBindingPolicy.SESSION)` to the new `spark.sql.legacy.jdbc.timeMapping.enabled` conf so it passes `SparkConfigBindingPolicySuite`, and add a test case asserting the escape hatch has no effect when `spark.sql.timeType.enabled` is false. Co-authored-by: Isaac
shrirangmhalgi
left a comment
shrirangmhalgi
left a comment
There was a problem hiding this comment.
Thanks for the followup @cloud-fan. LGTM - the escape-hatch semantics are correct: isTimeTypeEnabled && !legacyJdbcTimeMappingEnabled preserves the existing behavior when either flag disables TimeType. Verified the 3-way test matrix covers all combinations.
Note: per-dialect getCatalystType overrides run before the default mapper (JdbcUtils.getSchema line 335), so they need their own legacyJdbcTimeMappingEnabled check for the escape hatch to apply. I'll include this in #56904 (PostgreSQL TIME dialect) and other followup PRs once this PR merges.
3 participants
What changes were proposed in this pull request?
SPARK-57555 made the built-in JDBC data source read SQL
TIMEcolumns asTimeType(instead of the legacyTimestampType) whenspark.sql.timeType.enabledis true. That is a schema-inference change for existing JDBC reads: a column that previously came back asTimestampTypenow comes back asTimeType, with no DDL change by the user.This PR adds an internal escape-hatch conf,
spark.sql.legacy.jdbc.timeMapping.enabled(defaultfalse), following the existingspark.sql.legacy.<db>.*Mapping.enabledfamily. When set to true, JDBCTIMEcolumns keep the legacyTimestampTypemapping even whenspark.sql.timeType.enabledis true. It has no effect whenspark.sql.timeType.enabledis false.Why are the changes needed?
spark.sql.timeType.enabledis a single global switch that governsTIMEsupport across all of Spark. Enabling it flips JDBCTIMEreads fromTimestampTypetoTimeTypein one step, which can silently break workloads that depend on the old mapping (downstream casts, comparisons, stored schemas, serialization). The escape hatch lets such workloads opt back into the legacy JDBC behavior without having to keep the entireTIMEtype disabled.Does this PR introduce any user-facing change?
No behavioral change by default. The new conf defaults to
false, preserving SPARK-57555 behavior. When explicitly set totrue(andspark.sql.timeType.enabledistrue), JDBCTIMEcolumns read asTimestampTypeas they did before SPARK-57555.How was this patch tested?
Added a unit test in
JDBCSuiteasserting that, withspark.sql.timeType.enabled=true, the column reads asTimestampTypewhen the escape hatch is on and asTimeTypewhen it is off.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)